Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Allow reusing the same operator to speed up tests #2068

Merged
merged 18 commits into from
Apr 23, 2023
Merged

refactor: Allow reusing the same operator to speed up tests #2068

merged 18 commits into from
Apr 23, 2023

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Apr 22, 2023 •
edited
Loading

This PR refactor the behavior tests to allow using the same operator.

@Xuanwo
Fix scan root
f790859 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix debug for cap
d4dcb6d 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
avoid conflict
7fea57f 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix fs
14f13ca 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Allow retry content incomplete
fb78756 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Allow retry for copy and rename
9d82f7c 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
FIx list tests
3a0c426 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Fix ensure root
50ab64e 
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo
Allow retry for redis error
df61388 
Signed-off-by: Xuanwo <[email protected]>
@suyanhanx
Copy link
Member

I think the benefits are not significant, and it adds to the mental burden of maintainers. 👀

@Xuanwo
Copy link
Member Author

Xuanwo commented Apr 22, 2023 •
edited
Loading

I think the benefits are not significant, and it adds to the mental burden of maintainers. 👀

  • tests are faster
  • tests are happened on the same operator (like real use cases)
  • more hidden errors are exposed (I fixed at least 5 bugs in this PR)
  • easier to add new test suite

I believe the benefits is good enough.

@suyanhanx
Copy link
Member

I think the benefits are not significant, and it adds to the mental burden of maintainers. 👀

  • tests are faster
  • tests are happened on the same operator (like real use cases)
  • more hidden errors are exposed (I fixed at least 5 bugs in this PR)
  • easier to add new test suite

I believe the benefits is good enough.

If it's just for the sake of speed, I don't think the benefits are significant.
Adding those you mentioned would be sufficient.

suyanhanx
suyanhanx reviewed Apr 22, 2023
View reviewed changes
core/src/layers/retry.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo requested a review from suyanhanx April 22, 2023 19:33
suyanhanx
suyanhanx reviewed Apr 22, 2023
View reviewed changes
Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

Are there any points need to pay attention to? Please update them in the behavior tests' readme.

@Xuanwo
Copy link
Member Author

Xuanwo commented Apr 23, 2023

Are there any points need to pay attention to? Please update them in the behavior tests' readme.

No.

suyanhanx
suyanhanx approved these changes Apr 23, 2023
View reviewed changes
Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@Xuanwo Xuanwo merged commit ca53000 into main Apr 23, 2023
@Xuanwo Xuanwo deleted the refactor-behavior-tests branch April 23, 2023 03:01
@Xuanwo Xuanwo mentioned this pull request Apr 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suyanhanx suyanhanx suyanhanx approved these changes

@PsiACE PsiACE Awaiting requested review from PsiACE PsiACE is a code owner

@ClSlaid ClSlaid Awaiting requested review from ClSlaid

@sundy-li sundy-li Awaiting requested review from sundy-li

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Xuanwo @suyanhanx